Skip to content

Only update PrivilegesEvaluator configuration when the base config objects have actually changed.#6037

Draft
nibix wants to merge 2 commits intoopensearch-project:mainfrom
nibix:selective-role-updates
Draft

Only update PrivilegesEvaluator configuration when the base config objects have actually changed.#6037
nibix wants to merge 2 commits intoopensearch-project:mainfrom
nibix:selective-role-updates

Conversation

@nibix
Copy link
Collaborator

@nibix nibix commented Mar 26, 2026

Description

This changes PrivilegesConfiguration config change listening logic to only update the PrivilegesEvaluator and DLS/FLS configuration if the underlying configuration objects actually have been changed since the last time.

This can provide a small performance benefit, as for clusters with many roles and indices, the config updates can be costly (a few seconds for very large environments).

  • Category: Performance Enchancements
  • Why these changes are required?
  • What is the old behavior before changes and new behavior after changes? No behavioral changes.

Issues Resolved

Fixes #5999

Testing

  • Existing int tests

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@nibix
Only update PrivilegesEvaluator configuration when the base config ob…
df9aaf3 
…jects have actually changed.

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
@nibix
Changelog
004a9ca 
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
nibix
nibix commented Mar 26, 2026
View reviewed changes
src/main/java/org/opensearch/security/privileges/PrivilegesConfiguration.java
public CompiledRoles compiledRoles() {
return this.compiledRoles.get();
}

Copy link
Collaborator Author

@nibix nibix Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code was already unused before, removed to facilitate changes.

@nibix nibix mentioned this pull request Mar 26, 2026
Open
3 tasks
@cwperks
Copy link
Member

cwperks commented Mar 26, 2026

@nibix FYI test failures on the PR. Are you pushing a fix for these?


Suite: Test class org.opensearch.security.configuration.ConfigurationRepositoryTest
  2> 2026-03-26T13:14:58.773251697Z Test worker INFO Starting configuration org.apache.logging.log4j.core.config.properties.PropertiesConfiguration@402d45c6...
  2> 2026-03-26T13:14:58.779861023Z Test worker INFO Configuration org.apache.logging.log4j.core.config.properties.PropertiesConfiguration@402d45c6 started.
  2> 2026-03-26T13:14:58.783452249Z Test worker INFO Stopping configuration org.apache.logging.log4j.core.config.DefaultConfiguration@1d86b636...
  2> 2026-03-26T13:14:58.784409161Z Test worker INFO Configuration org.apache.logging.log4j.core.config.DefaultConfiguration@1d86b636 stopped.
  2> java.lang.AssertionError: 
    Expected: is not <SecurityDynamicConfiguration [seqNo=-1, primaryTerm=-1, ctype=CONFIG, version=2, centries={}, getImplementingClass()=class org.opensearch.security.securityconf.impl.v7.ConfigV7]>
         but: was <SecurityDynamicConfiguration [seqNo=-1, primaryTerm=-1, ctype=CONFIG, version=2, centries={}, getImplementingClass()=class org.opensearch.security.securityconf.impl.v7.ConfigV7]>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.opensearch.security.configuration.ConfigurationRepositoryTest.getConfiguration_withInvalidConfigurationShouldReturnNewEmptyConfigurationObject(ConfigurationRepositoryTest.java:343)

Tests with failures:
 - org.opensearch.security.securityconf.impl.SecurityDynamicConfigurationTest.deepClone_shouldReturnNewObject
 - org.opensearch.security.configuration.ConfigurationRepositoryTest.getConfiguration_withInvalidConfigurationShouldReturnNewEmptyConfigurationObject

@nibix
Copy link
Collaborator Author

nibix commented Mar 26, 2026

This are easy to fix ... i am debugging at the moment these:

RollbackVersionApiTest > testRollbackWithoutEnoughVersions_shouldFail FAILED
    java.lang.AssertionError: 
    Expected: is <404>
         but: was <200>
        at __randomizedtesting.SeedInfo.seed([E3FEB377C5119CA2:A95F4D4AA34D797E]:0)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.opensearch.security.dlic.rest.api.RollbackVersionApiTest.testRollbackWithoutEnoughVersions_shouldFail(RollbackVersionApiTest.java:154)

RollbackVersionApiTest > testRollbackToPreviousVersion_success FAILED
    java.lang.AssertionError: 
    Expected: a string containing "config rolled back to version v1"
         but: was "{
      "status" : "OK",
      "message" : "config rolled back to version v2"
    }"
        at __randomizedtesting.SeedInfo.seed([E3FEB377C5119CA2:C4832FC52AB61458]:0)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.opensearch.security.dlic.rest.api.RollbackVersionApiTest.testRollbackToPreviousVersion_success(RollbackVersionApiTest.java:96)

There I am not yet clear on the cause, taking back to draft

@nibix nibix marked this pull request as draft March 26, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cwperks cwperks Awaiting requested review from cwperks cwperks is a code owner

@DarshitChanpura DarshitChanpura Awaiting requested review from DarshitChanpura DarshitChanpura is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@finnegancarroll finnegancarroll Awaiting requested review from finnegancarroll finnegancarroll is a code owner

@RyanL1997 RyanL1997 Awaiting requested review from RyanL1997 RyanL1997 is a code owner

@reta reta Awaiting requested review from reta reta is a code owner

@shikharj05 shikharj05 Awaiting requested review from shikharj05 shikharj05 is a code owner

@willyborankin willyborankin Awaiting requested review from willyborankin willyborankin is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Avoid reconstructing RoleBasedActionPrivileges when security configs other than roles and actiongroups change

2 participants

@nibix @cwperks